Refactor contributors list & fetch wrapper [i18nIgnore] #12530
Conversation
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
I confirmed this was a token issue and updating our token fixed deploys. Nonetheless, some good action items we should look at coming out of this:
|
github-actions
bot
added
i18n
Lunaria Status Overview🌑 This pull request will not trigger status changes. Learn moreLunaria automatically ignores changes on specific PRs by adding a ignored keyword in its title. Found: You can change this by either removing the keyword above from the PR's title, or modifying the Tracked FilesNote The notes below indicate what would happen if the pull request is merged when triggering status changes. Since a ignored keyword was found in the PR's title, the status changes indicated below won't be applied.
Warnings reference
|
HiDeoo
changed the title
|
Updated the PR based on your recommendations:
|
There was a problem hiding this comment.
This looks fantastic @HiDeoo! +1 to the retry logic and nice work moving this to a workflow. Great to see it cleaned up.
We only store the data we really need (e.g. not the number of contributions which will make diffs a bit easier — I guess showing (and storing) contributors alphabetically rather than number of contributions would make this even easier if we wanted).
Off the top of my head, we ultimately render in order from most contributions to least, so I guess we have two options here:
- Store pre-sorted contributors — only requires storing usernames.
- Store alphabetically sorted contributors — would require storing username + contribution count.
I don’t have strong opinions on which is better. Option 1 I guess will result in a slightly messier diff as entries move up and down, while option 2 will be more stable but all contributors in a week will show up as their contribution count changes.
One thought though: we’re not using the id field right? Why not make the login value the id and just use that? So with the current approach the file would look like:
[
{
"id": "jsparkdev"
},
{
"id": "ArmandPhilippot"
},
{
"id": "sarah11918"
},
// etc.
]Or if we included a count:
[
{
"id": "ArmandPhilippot",
"count": 100
},
{
"id": "jsparkdev",
"count": 1000
},
{
"id": "sarah11918",
"count": 500
},
// etc.
]There’s also another option, which is to not bother with content collections for this and just store:
[
"jsparkdev",
"ArmandPhilippot",
"sarah11918",
// etc.
]The component could just import the JSON file for this one — we don’t really need the content collection features.
We actually do for the profile picture that's why I went with both of these fields being stored. |
There was a problem hiding this comment.
We actually do for the profile picture that's why I went with both of these fields being stored.
Ohhhh, yes. I actually thought I checked that but I think I was looking at the other attributes that changed in the diff instead of the src 😁
OK, then I’m happy with the current version. We can always see if how our first few diffs look and switch to alphabetical later if we need.
HiDeoo
added
the
hacktoberfest-accepted
3 participants
Description (required)
The issue has been found and is related to a request to the GitHub API to fetch contributors returning a 403 status code.
The issue will not be fixed in this PR but I'm keeping the investigation notes here for reference and also as a reminder to refactor this part of the code to add more logging when fetching failures happen and also revisit the retry logic (the default settings exceeds the Netlify build time limit).
renderToString) "Contribute to Astro"Bad response for https://api.github.com/repos/withastro/docs/contributors (403): ForbiddenRelated issues & labels (optional)